-
Notifications
You must be signed in to change notification settings - Fork 10.4k
59462 OIDC par failure #61947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
59462 OIDC par failure #61947
Conversation
@dotnet-policy-service agree company="Tyler Technologies" |
Would love feedback from @josephdecock Also, I'm not sure if this should go into main, but I'd like to see it show up in at least the 10.0 release (it's probably too late to get it into 9). |
If there anything else I need to do process-wise to make this PR ready for review, please let me know. This is my first contribution and I might have accidentally missed a step. |
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
/azp run |
Commenter does not have sufficient privileges for PR 61947 in repo dotnet/aspnetcore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new OIDC event OnPushAuthorizationFailed
to handle PAR (Pushed Authorization Request) failures during the challenge phase. When a PAR request fails with an exception, applications can now handle the response gracefully instead of receiving an unhandled middleware exception.
Key changes:
- New
PushedAuthorizationFailedContext
class to provide context for PAR failures - New
OnPushAuthorizationFailed
event inOpenIdConnectEvents
- Exception handling logic in
OpenIdConnectHandler
to catch PAR failures and fire the event - Test coverage for the new failure handling functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
OpenIdConnectChallengeTests.cs |
Adds test to verify PAR failure event handling works correctly |
PublicAPI.Unshipped.txt |
Exposes new public API members for the failure context and event |
OpenIdConnectHandler.cs |
Wraps PAR logic in try-catch to fire failure event when exceptions occur |
LoggingExtensions.cs |
Adds logging for handled PAR failure responses |
PushedAuthorizationFailedContext.cs |
New context class providing exception details and handled flag |
OpenIdConnectEvents.cs |
Adds the new failure event to the events collection |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
src/Security/Authentication/OpenIdConnect/src/Events/PushedAuthorizationFailedContext.cs
Outdated
Show resolved
Hide resolved
@@ -74,7 +74,7 @@ internal static partial class LoggingExtensions | |||
[LoggerMessage(37, LogLevel.Debug, "The UserInformationReceived event returned Skipped.", EventName = "UserInformationReceivedSkipped")] | |||
public static partial void UserInformationReceivedSkipped(this ILogger logger); | |||
|
|||
[LoggerMessage(57, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")] | |||
[LoggerMessage(60, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event ID was changed from 57 to 60, but this appears to be an existing log message that should maintain its original ID. This change could break logging consistency and any code that depends on the specific event ID.
[LoggerMessage(60, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")] | |
[LoggerMessage(57, LogLevel.Debug, "The PushAuthorization event handled client authentication", EventName = "PushAuthorizationHandledClientAuthentication")] |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were two values set for 57, which I assumed to be a mistake from a previous commit. I updated this to be the next unique number at the time. I am not sure if that was the correct move.
break; | ||
} | ||
} | ||
catch(Exception exception) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after catch
keyword. Should be catch (Exception exception)
according to C# formatting conventions.
Copilot uses AI. Check for mistakes.
…horizationFailedContext.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add OIDC event to allow caller to react to OIDC PAR failure during challenge phase
Description
Add new OIDC OnPushAuthorizationFailed, new context for this event, logic to fire event, tests.
Problem
When a validation failure occurs during a PAR request (ex. the request includes an invalid client_id), an OpenIdConnectProtocolException is thrown. This exception bubbles up as an unhandled middleware exception.
Please see issue #59462 for slightly more details.
Goal
We need to give the application the ability to handle the response when a PAR request fails during the challenge phase. Since an exception during the challenge phase bubbles up as a middleware exception, it is difficult for the application to respond. By including a specific OIDC event, the application has the opportunity to redirect the browser to a user-friendly error page.
Example of a web app utilizing this new feature
Fixes #59462